Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: move fixtures, expose them via load func #353

Merged
merged 6 commits into from
Mar 9, 2023
Merged

chore: move fixtures, expose them via load func #353

merged 6 commits into from
Mar 9, 2023

Conversation

mikeldking
Copy link
Contributor

@mikeldking mikeldking commented Mar 9, 2023 •
edited
Loading

resolves #287

import phoenix as px

primary_dataset, reference_dataset = px.load_datasets("sentiment_classification_language_drift")
session = px.launch_app(primary_dataset, reference_dataset)
session.view()

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

axiomofjoy
axiomofjoy approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@axiomofjoy axiomofjoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Another option is to return a DatasetDict object. That way if a user does not know the return type on the load_datasets method, they can just inspect a dictionary (easier to inspect in Jupyter than a tuple).

Screenshot 2023-02-27 at 6 00 55 PM

@axiomofjoy
Copy link
Contributor

LGTM. Another option is to return a DatasetDict object. That way if a user does not know the return type on the load_datasets method, they can just inspect a dictionary (easier to inspect in Jupyter than a tuple).

Screenshot 2023-02-27 at 6 00 55 PM

Might also be preferable in case we want to support fixtures with only a primary dataset down the line.

axiomofjoy
axiomofjoy reviewed Mar 9, 2023
View reviewed changes
src/phoenix/datasets/fixtures.py Outdated

def load_datasets(fixture_name: str) -> Tuple[Dataset, Dataset]:
"""
Loads the primary and reference datasets for a fixture.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: I'd remove the terminology "fixture" from everything user-facing. I think this term is more likely to be familiar to a software engineer than a data scientist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah no doubt.

mikeldking
mikeldking commented Mar 9, 2023
View reviewed changes
src/phoenix/datasets/fixtures.py

Parameters
----------
use_case: str
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@axiomofjoy switched to use_case

src/phoenix/datasets/fixtures.py
Comment on lines +255 to +260
@dataclass(frozen=True)
class DatasetDict(Dict[str, Dataset]):
"""A dictionary of datasets, split out by dataset type (primary, reference)."""

primary: Dataset
reference: Dataset
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mimic hugging face "split"

@mikeldking mikeldking merged commit e80a57c into main Mar 9, 2023
@mikeldking mikeldking deleted the chore/mv-datasets branch March 9, 2023 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axiomofjoy axiomofjoy axiomofjoy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move fixtures to datasets
2 participants
@mikeldking @axiomofjoy